PDO_Firebird: Supported Firebird 4.0 datatypes #14897

sim1984 · 2024-07-10T08:33:55Z

Implementation #14896

sim1984 · 2024-07-10T09:40:00Z

This patch requires Firebird 4.0 header files to be present in the build system, since type identifiers SQL_INT128, SQL_DEC16, SQL_DEC34, SQL_TIMESTAMP_TZ, SQL_TIME_TZ are required.

SakiTakamachi · 2024-07-10T10:18:57Z

Thank you, I will check this as soon as possible.

cmb69 · 2024-07-10T12:34:19Z

This patch requires Firebird 4.0 header files to be present in the build system, […]

For Windows, the firebird-*.zips (and the respective series files) would need to be updated on https://downloads.php.net/~windows/php-sdk/deps/. Probably a good idea anyway (i.e. regardless of this PR), at least if we can assume that the includes and lib are downward compatible to older FB servers.

sim1984 · 2024-07-10T12:44:30Z

fbclient is exactly compatible. Even fbclient version 5.0 works fine with Firebird 2.5. As for the header files, I don’t see any problems here either. My patch just uses a few constants declared as #define. That is, functions specific to the new fbclient are not used.

SakiTakamachi

The changes look good to me, but need to get the version of firebird in CI on windows to 4 (I created a PR)
#14912

I don't understand why the LINUX_X64_RELEASE_NTS test is failing... (It should be using 4.02)

SakiTakamachi · 2024-07-11T05:03:40Z

ext/pdo_firebird/firebird_driver.c

+static void set_coercing_data_type(XSQLDA* sqlda)
+{
+    /* Data types introduced in Firebird 4.0 are difficult to process using the Firebird Legacy API. */ 
+	/* These data types include DECFLOAT(16), DECFLOAT(34), INT128 (NUMERIC/DECIMAL(38, x)), */
+	/* TIMESTAMP WITH TIME ZONE, and TIME WITH TIME ZONE. In any case, at least the first three data types */
+	/* can only be mapped to strings. The last two too, but for them it is potentially possible to set */
+	/* the display format, as is done for TIMESTAMP. This function allows you to ensure minimal performance */ 
+	/* of queries if they contain columns and parameters of the above types. */
+	unsigned int i;
+	short dtype;
+	short nullable;
+	XSQLVAR* var;
+	for (i=0, var = sqlda->sqlvar; i < sqlda->sqld; i++, var++) {
+        dtype = (var->sqltype & ~1); /* drop flag bit  */
+		nullable = (var->sqltype & 1); 
+		switch(dtype) {
+			case SQL_INT128:
+			    var->sqltype = SQL_VARYING + nullable;
+				var->sqllen = 46;
+				var->sqlscale = 0;
+			    break;
+			case SQL_DEC16:
+			    var->sqltype = SQL_VARYING + nullable;
+				var->sqllen = 24;
+			    break;
+			case SQL_DEC34:
+			    var->sqltype = SQL_VARYING + nullable;
+				var->sqllen = 43;
+			    break;
+			case SQL_TIMESTAMP_TZ:
+			    var->sqltype = SQL_VARYING + nullable;
+				var->sqllen = 58;
+			    break;
+			case SQL_TIME_TZ:
+			    var->sqltype = SQL_VARYING + nullable;
+				var->sqllen = 46;
+			    break;														
+		}
+	}	
+}


Please standardize indentation to TAB.

Suggested change

static void set_coercing_data_type(XSQLDA* sqlda)

{

/* Data types introduced in Firebird 4.0 are difficult to process using the Firebird Legacy API. */

/* These data types include DECFLOAT(16), DECFLOAT(34), INT128 (NUMERIC/DECIMAL(38, x)), */

/* TIMESTAMP WITH TIME ZONE, and TIME WITH TIME ZONE. In any case, at least the first three data types */

/* can only be mapped to strings. The last two too, but for them it is potentially possible to set */

/* the display format, as is done for TIMESTAMP. This function allows you to ensure minimal performance */

/* of queries if they contain columns and parameters of the above types. */

unsigned int i;

short dtype;

short nullable;

XSQLVAR* var;

for (i=0, var = sqlda->sqlvar; i < sqlda->sqld; i++, var++) {

dtype = (var->sqltype & ~1); /* drop flag bit */

nullable = (var->sqltype & 1);

switch(dtype) {

case SQL_INT128:

var->sqltype = SQL_VARYING + nullable;

var->sqllen = 46;

var->sqlscale = 0;

break;

case SQL_DEC16:

var->sqltype = SQL_VARYING + nullable;

var->sqllen = 24;

break;

case SQL_DEC34:

var->sqltype = SQL_VARYING + nullable;

var->sqllen = 43;

break;

case SQL_TIMESTAMP_TZ:

var->sqltype = SQL_VARYING + nullable;

var->sqllen = 58;

break;

case SQL_TIME_TZ:

var->sqltype = SQL_VARYING + nullable;

var->sqllen = 46;

break;

}

}

}

static void set_coercing_data_type(XSQLDA* sqlda)

{

/* Data types introduced in Firebird 4.0 are difficult to process using the Firebird Legacy API. */

/* These data types include DECFLOAT(16), DECFLOAT(34), INT128 (NUMERIC/DECIMAL(38, x)), */

/* TIMESTAMP WITH TIME ZONE, and TIME WITH TIME ZONE. In any case, at least the first three data types */

/* can only be mapped to strings. The last two too, but for them it is potentially possible to set */

/* the display format, as is done for TIMESTAMP. This function allows you to ensure minimal performance */

/* of queries if they contain columns and parameters of the above types. */

unsigned int i;

short dtype;

short nullable;

XSQLVAR* var;

for (i=0, var = sqlda->sqlvar; i < sqlda->sqld; i++, var++) {

dtype = (var->sqltype & ~1); /* drop flag bit */

nullable = (var->sqltype & 1);

switch(dtype) {

case SQL_INT128:

var->sqltype = SQL_VARYING + nullable;

var->sqllen = 46;

var->sqlscale = 0;

break;

case SQL_DEC16:

var->sqltype = SQL_VARYING + nullable;

var->sqllen = 24;

break;

case SQL_DEC34:

var->sqltype = SQL_VARYING + nullable;

var->sqllen = 43;

break;

case SQL_TIMESTAMP_TZ:

var->sqltype = SQL_VARYING + nullable;

var->sqllen = 58;

break;

case SQL_TIME_TZ:

var->sqltype = SQL_VARYING + nullable;

var->sqllen = 46;

break;

}

}

}

Also, there is an EMPTY_SWITCH_DEFAULT_CASE() macro that can be used for the default case where the switch does nothing.

If you insert this macro, the driver crashes as soon as the dtype goes beyond the cases described in case. In my case, for other types you just need to do nothing.

SakiTakamachi · 2024-07-11T05:05:33Z

ext/pdo_firebird/firebird_driver.c

+		}
+	}	
+}
+/* }}} */


Probably not necessary /* }}} */

SakiTakamachi · 2024-07-11T05:06:25Z

ext/pdo_firebird/firebird_driver.c

+#if FB_API_VER >= 40
+        /* set coercing a data type */
+		set_coercing_data_type(&S->out_sqlda);
+#endif


Ditto, please use consistent indentation.

SakiTakamachi · 2024-07-11T05:06:35Z

ext/pdo_firebird/firebird_driver.c

+#if FB_API_VER >= 40
+			/* set coercing a data type */
+		    set_coercing_data_type(S->in_sqlda);
+#endif			


SakiTakamachi · 2024-07-11T05:38:56Z

ext/pdo_firebird/tests/fb4_datatypes.phpt

+--EXTENSIONS--
+pdo_firebird
+--SKIPIF--
+<?php require('skipif.inc'); ?>


Versions lower than 4 may want to skip this test

How to set this restriction?

$dbh->getAttribute(PDO::ATTR_SERVER_VERSION);

Gets the server version, but it looks like this

Firebird/Linux/AMD/Intel/x64 (access method), version "LI-V5.0.1.1391 Firebird 5.0 faca113" Firebird/Linux/AMD/Intel/x64 (remote server), version "LI-V5.0.1.1391 Firebird 5.0 faca113/tcp (DESKTOP-E3INAFT)/P18:C" Firebird/Linux/AMD/Intel/x64 (remote interface), version "LI-V5.0.1.1391 Firebird 5.0 faca113/tcp (DESKTOP-E3INAFT)/P18:C" on disk structure version 13.1

You also need to use a regular expression to make this information simple.

On the other hand, the engine version can be obtained by request

select rdb$get_context('SYSTEM', 'ENGINE_VERSION') from rdb$database

There is only a version number. But in any case, the server version can only be found after connecting to the database.

Create a separate function that returns the engine version?

What matters here is the client version, not the server version. So, could you please try the following constant?

PDO::ATTR_CLIENT_VERSION

It also returns a complex version of the client

LI-V6.3.1.1391 Firebird 5.0

The success of this test depends less on the client (although it does too) and more on the Firebird header files.

#if FB_API_VER >= 40 ... #endif

FB_API_VER is defined in ibase.h, and if the header file is from Firebird 3.0, this branch simply will not end up in the binary file when compiled.

Are you assuming the case that there is a difference between the client version and FB_API_VER? That's certainly a possible problem.

Now, probably the simplest solution here is to be able to retrieve FB_API_VER. In fact, this is useful for users when debugging on their own.

Could you please wait a moment while I open the PR to add?

Yes, sure. The API version is essentially the version of the header files. And the client version is the version of fbclient.dll or libfbclient.so. They are generally independent.

What is the point of the patch? After preparing query, the server says I have these types of columns and input parameters. We cannot process some of them, so in response we tell the server, instead of these types, return VARCHAR(N) to us. And this part, in theory, can generally work independently of the client (I have not tested).

Now, probably the simplest solution here is to be able to retrieve FB_API_VER. In fact, this is useful for users when debugging on their own.

Could you please wait a moment while I open the PR to add?

OK. I also think this will be useful.

Opened PR
#14916

SakiTakamachi · 2024-07-11T05:39:10Z

ext/pdo_firebird/tests/fb4_datatypes_params.phpt

+--EXTENSIONS--
+pdo_firebird
+--SKIPIF--
+<?php require('skipif.inc'); ?>


SakiTakamachi · 2024-07-17T11:51:31Z

@sim1984
I made you wait. Now that my PR has been merged into master, please incorporate it into your branch.
You can get the attribute value and skip the test :)

edit:
Note that attribute constants are implemented in subclasses.
e.g.

$db = new PDO();
$ver = $db->getAttribute(Pdo\Firebird::ATTR_API_VERSION);

sim1984 · 2024-07-17T19:20:16Z

I added API version checking. But I don’t like that the API version can be checked only after connecting to the database.

sim1984 · 2024-07-17T20:04:27Z

I have no idea how to skip the test before connecting to the database. If you just start connect it in "--SKIPIF--" section. But now at least it’s clear why the test fails on Linux x64. The header files there are not from Firebird 4.0.

sim1984 · 2024-07-17T20:11:22Z

Maybe add a driver-specific function public static function Pdo\Firebird::GetApiVersion(): int?
For example, Pdo\Pgsql contains such functions.

cmb69 · 2024-07-17T20:15:07Z

But I don’t like that the API version can be checked only after connecting to the database.

Indeed, that doesn't make much sense, since it is not related to the Firebird server at all. Not sure if there is precedent in PDO, but can't we just have a constant which contains the API version (something like Pdo\Firebird::API_VERSION)?

PS: or maybe instead of a constant a static method on Pdo\Firebird.
PPS: haven't seen @sim1984's latest comment.

SakiTakamachi · 2024-07-17T23:17:40Z

PDO constants have a very strong impression of being attribute keys, and since the API version is int and the value is small, if implement it as a constant, it may have the same value as the PDO core attribute key. Therefore, there is a concern that bugs may occur due to user misunderstandings.

Therefore, if want to make this more convenient, I think it makes the most sense to implement it as a static method.
My implementation was the "least effort" way, so I'll try reimplementing it using static methods.

I think it's very meaningful to be able to have this kind of discussion before release :)

SakiTakamachi · 2024-07-17T23:45:56Z

done
#15004

SakiTakamachi · 2024-07-18T00:16:43Z

I have no idea how to skip the test before connecting to the database. If you just start connect it in "--SKIPIF--" section. But now at least it’s clear why the test fails on Linux x64. The header files there are not from Firebird 4.0.

Right. Since the one from Ubuntu is installed, it's probably 3.0.
Ideally, I would like to upgrade to 4.0, but personally, I think it can be done after than this PR. (nightly may also be fine)

SakiTakamachi · 2024-07-18T02:19:54Z

@sim1984

The change to replace attribute value retrieval with static method retrieval has been merged into master :)

sim1984 · 2024-07-18T06:23:32Z

done

SakiTakamachi · 2024-07-18T12:35:29Z

@sim1984

Thanks, I will test this later on my local and confirm!

SakiTakamachi

@sim1984
Sorry for the late confirmation.
It looks mostly good, but the format of the date in the test expected value is different in my environment, causing the test to fail.

There doesn't seem to be a way to easily specify this, so could you please fix the expected value for a test that doesn't care about the date format?

SakiTakamachi · 2024-07-22T14:19:53Z

ext/pdo_firebird/tests/fb4_datatypes.phpt

+--EXPECT--
+{
+    "I64": 15,
+    "I128": "15",
+    "N": "123.97",
+    "N2": "123.97",
+    "TS": "2024-05-04 12:59:34",
+    "TS_TZ": "2024-05-04 12:59:34.2390 Europe\/Moscow",
+    "T": "12:59:34",
+    "T_TZ": "12:59:34.2390 Europe\/Moscow",
+    "DF16": "1.128",
+    "DF34": "1.128"
+}


In my environment, the date format is probably different than yours.

005- "TS_TZ": "2024-05-04 12:59:34.2390 Europe\/Moscow", 006+ "TS_TZ": "04-MAY-2024 12:59:34.2390 Europe\/Moscow",

I thought about how to specify the format, but I don't want to do that yet because I think it would be quite complicated.

Therefore, could you please change your test expectations to:

Suggested change

--EXPECT--

{

"I64": 15,

"I128": "15",

"N": "123.97",

"N2": "123.97",

"TS": "2024-05-04 12:59:34",

"TS_TZ": "2024-05-04 12:59:34.2390 Europe\/Moscow",

"T": "12:59:34",

"T_TZ": "12:59:34.2390 Europe\/Moscow",

"DF16": "1.128",

"DF34": "1.128"

}

--EXPECTF--

{

"I64": 15,

"I128": "15",

"N": "123.97",

"N2": "123.97",

"TS": "2024-05-04 12:59:34",

"TS_TZ": "%s 12:59:34.2390 Europe\/Moscow",

"T": "12:59:34",

"T_TZ": "12:59:34.2390 Europe\/Moscow",

"DF16": "1.128",

"DF34": "1.128"

}

SakiTakamachi · 2024-07-22T14:20:01Z

ext/pdo_firebird/tests/fb4_datatypes_params.phpt

+--EXPECT--
+{
+    "I128": "12",
+    "N1": "12.34",
+    "N2": "12.34",
+    "TS_TZ": "2024-05-04 12:59:34.2390 Europe\/Moscow",
+    "T_TZ": "12:59:00.0000 Europe\/Moscow",
+    "DF16": "12.34",
+    "DF34": "12.34"
+}


SakiTakamachi · 2024-07-22T15:15:12Z

@sim1984

APIs like isc_decode_XXX don't exist for new column types, so there doesn't seem to be an easy way to work with them.

Do you know anything about this? If I can't find any clues here, I'm thinking of asking upstream.

sim1984 · 2024-07-22T15:58:22Z

@sim1984

APIs like isc_decode_XXX don't exist for new column types, so there doesn't seem to be an easy way to work with them.

Do you know anything about this? If I can't find any clues here, I'm thinking of asking upstream.

APIs like isc_decode_XXX don't really exist for types with time zones. They are in the new object-oriented API. It is mainly for C++, but CLOOP files can be generated for C. But I wouldn’t want to drag out quite large implementations of the new C API for the sake of a small improvement. It may be possible to isolate only the necessary functionality from there. Need to try.

SakiTakamachi · 2024-07-22T16:10:03Z

@sim1984

Thanks, I don't intend to think about this on a grand scale either.

At the very least, I think this PR can be merged as soon as the tests are fixed.

This means that the decoding issue can be addressed in a follow-up PR. (Of course, there are cases where we can choose not to do that)

There is currently no way to set the output format for time zone types, so the %s substitution is used in the expected output.

SakiTakamachi · 2024-07-23T13:37:42Z

@sim1984
Thanks!
I merged this on 00e4588

As a follow-up to the commit which introduced support for Firebird 4.0+ data types[1], we add support for formats for types with time zones. Since this uses the newer Firebird C++ API, pdo_firebird now requires a C++ compiler to be built. [1] <#14897> Co-authored-by: Christoph M. Becker <cmbecker69@gmx.de> Closes GH-15230.

PDO_Firebird: Supported Firebird 4.0 datatypes

950de97

sim1984 requested a review from SakiTakamachi as a code owner July 10, 2024 08:33

github-actions bot added the Extension: pdo_firebird label Jul 10, 2024

cmb69 linked an issue Jul 10, 2024 that may be closed by this pull request

Add support Firebird 4.0+ datatypes #14896

Closed

SakiTakamachi reviewed Jul 11, 2024

View reviewed changes

sim1984 added 2 commits July 11, 2024 09:25

tab indent

546af78

add EMPTY_SWITCH_DEFAULT_CASE()

af79543

SakiTakamachi mentioned this pull request Jul 11, 2024

ext/pdo_firebird: Added Pdo\Firebird::ATTR_API_VERSION #14916

Closed

sim1984 added 2 commits July 17, 2024 19:53

Merge branch 'php:master' into master

a70cf1b

skip if firebird api version < 40

0e056cb

SakiTakamachi mentioned this pull request Jul 17, 2024

ext/pdo_firebird: Added getApiVersion() and removed from getAttribute() #15004

Merged

sim1984 added 2 commits July 18, 2024 08:16

Merge branch 'php:master' into master

841a1e5

usage Pdo\Firebird::getApiVersion() in tests

4b71218

SakiTakamachi reviewed Jul 22, 2024

View reviewed changes

Tests fixed.

af31c84

There is currently no way to set the output format for time zone types, so the %s substitution is used in the expected output.

SakiTakamachi closed this in 00e4588 Jul 23, 2024

PDO_Firebird: Supported Firebird 4.0 datatypes #14897

PDO_Firebird: Supported Firebird 4.0 datatypes #14897

Uh oh!

Conversation

sim1984 commented Jul 10, 2024

Uh oh!

sim1984 commented Jul 10, 2024

Uh oh!

SakiTakamachi commented Jul 10, 2024

Uh oh!

cmb69 commented Jul 10, 2024

Uh oh!

sim1984 commented Jul 10, 2024

Uh oh!

SakiTakamachi left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SakiTakamachi Jul 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sim1984 Jul 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sim1984 Jul 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SakiTakamachi commented Jul 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sim1984 commented Jul 17, 2024

Uh oh!

sim1984 commented Jul 17, 2024

Uh oh!

sim1984 commented Jul 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cmb69 commented Jul 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SakiTakamachi commented Jul 17, 2024

Uh oh!

SakiTakamachi commented Jul 17, 2024

Uh oh!

SakiTakamachi commented Jul 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SakiTakamachi commented Jul 18, 2024

Uh oh!

sim1984 commented Jul 18, 2024

Uh oh!

SakiTakamachi commented Jul 18, 2024

Uh oh!

SakiTakamachi left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

SakiTakamachi left a comment •

edited

Loading

SakiTakamachi Jul 11, 2024 •

edited

Loading

sim1984 Jul 11, 2024 •

edited

Loading

sim1984 Jul 11, 2024 •

edited

Loading

SakiTakamachi commented Jul 17, 2024 •

edited

Loading

sim1984 commented Jul 17, 2024 •

edited

Loading

cmb69 commented Jul 17, 2024 •

edited

Loading

SakiTakamachi commented Jul 18, 2024 •

edited

Loading

SakiTakamachi left a comment •

edited

Loading